Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix FloatingBox merged refs being overwritten by ref prop in React 19 #3974

Merged
merged 4 commits into from
Dec 20, 2024

Conversation

thyhjwb6
Copy link
Collaborator

@thyhjwb6 thyhjwb6 commented Dec 19, 2024

Related issue

Fixes #3969

Overview

Fixes an issue when using React 19 which adds the ref as a prop. This was overwriting the merged refs because it is null.

Reason

The floating box for the DatePicker is positioned incorrectly.

Work carried out

  • Align existing tests with patterns
  • Add a test to cover positioning
  • Fix the ref issue

Test instructions

Follow the reproduction steps in the original issue.

Developer notes

A follow up to this (hopefully tomorrow) will be to replace React Popper with Floating UI.

Copy link

netlify bot commented Dec 19, 2024

Deploy Preview for storybook-navy-digital-mod-uk ready!

Name Link
🔨 Latest commit 00fe246
🔍 Latest deploy log https://app.netlify.com/sites/storybook-navy-digital-mod-uk/deploys/67653bbb88a8ad0007ce808e
😎 Deploy Preview https://deploy-preview-3974--storybook-navy-digital-mod-uk.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@thyhjwb6 thyhjwb6 self-assigned this Dec 19, 2024
@thyhjwb6 thyhjwb6 added Type: Bug Inconsistencies or issues which have caused a problem for users or implementors Package: react-component-library Package/code type Workstream: ADMS Related workstream labels Dec 19, 2024
@thyhjwb6 thyhjwb6 force-pushed the issue-3969/fix-floating-box-ref-rest branch from e0b7722 to 1362d86 Compare December 19, 2024 11:03
@thyhjwb6 thyhjwb6 changed the title [WIP] Issue 3969/fix floating box ref rest Fix FloatingBox merged refs being overwritten by ref prop in React 19 Dec 19, 2024
@thyhjwb6 thyhjwb6 marked this pull request as ready for review December 19, 2024 11:25
Copy link
Collaborator

@m7kvqbe1 m7kvqbe1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

data-testid="floating-box"
{...attributes.popper}
{...rest}
/*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this is really nitpicky but could we change the comment style to:

//
//
//

When using React 19, ref is a prop and was being overwritten with {...rest}.
Setup functions can be safely abstracted and reused across tests.
@thyhjwb6 thyhjwb6 force-pushed the issue-3969/fix-floating-box-ref-rest branch from 1362d86 to 00fe246 Compare December 20, 2024 09:41
@thyhjwb6 thyhjwb6 enabled auto-merge December 20, 2024 09:42
@thyhjwb6 thyhjwb6 merged commit e0482a2 into master Dec 20, 2024
20 checks passed
@thyhjwb6 thyhjwb6 deleted the issue-3969/fix-floating-box-ref-rest branch December 20, 2024 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: react-component-library Package/code type Type: Bug Inconsistencies or issues which have caused a problem for users or implementors Workstream: ADMS Related workstream
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect DatePicker FloatingCalendar positioning with React 19
2 participants